[fix] Guard missing skip_push_update_on_save in DeviceRegisterView#1331
[fix] Guard missing skip_push_update_on_save in DeviceRegisterView#1331MichaelUray wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThis change makes Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental changes further refine the implementation:
The implementation is clean, well-documented, and the optimization is safe. The fast-path correctly handles all edge cases (no config, not modified, stale cached modified state). Files Reviewed (2 files)
Reviewed by kimi-k2.5-0127 · 107,007 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/controller/views.py`:
- Line 197: The local import "from ..models import Config" in views.py is
redundant because Config is already loaded at module level via
load_model("config", "Config"); remove that local import and update any
references in the function/method where it appears to use the module-level
Config symbol instead (look for the local import and any subsequent usage within
that scope, e.g., in functions or class-based view methods).
- Around line 197-204: The code always does Config.objects.get(device=device) on
each checksum request causing an extra DB query; instead, read the cached device
object's config status first (use the cached device variable `device` and its
config status attribute—e.g., `device.config_status` or `device.config.status`
depending on your model) and only call `Config.objects.get(device=device)` when
that status indicates reconciliation is needed (e.g., status == "modified" or !=
"applied"); this limits DB hits to when a config change is possible while
preserving current behavior when reconciliation is required.
In `@openwisp_controller/config/tests/test_controller.py`:
- Line 279: The test file contains an unused import "PropertyMock" in the import
statement; remove the unused import from
openwisp_controller/config/tests/test_controller.py (the line importing
PropertyMock) so only required mocks are imported, ensuring no other references
to PropertyMock remain in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a894d0d-4898-407d-8184-d0de5278cf54
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (3)
openwisp_controller/config/tests/test_controller.py (1)
273-356: Well-structured test coverage for reconciliation behavior.The four test cases effectively cover:
- Reconciliation after grace period elapsed
- No change for already-applied configs
- No reconciliation within grace period
- Bypassing cached device state for fresh DB reads
openwisp_controller/config/controller/views.py (2)
541-549: Correct defensive guard for potentially missing method.The
getattr+callablepattern properly prevents theAttributeErrorthat was breaking factory-reset re-registration whenDevice.skip_push_update_on_save()is not implemented. This is a backward-compatible fix that will automatically work if the method is added later.
147-152: No action needed: Reconciliation signal does not trigger device push operations.The
config_status_changedsignal emitted byset_status_applied()(line 212) only fires theconfig_status_error_notificationhandler, which sends notifications exclusively when status is "error". Device push updates are triggered byconfig_modifiedandconfig_deactivatingsignals, not byconfig_status_changed. Reconciliation to "applied" status is therefore safe and does not cause redundant push attempts.
| and enough time has passed (grace period), the status should | ||
| be automatically reconciled to "applied". | ||
| """ | ||
| from unittest.mock import PropertyMock |
There was a problem hiding this comment.
Remove unused import PropertyMock.
This import is not used anywhere in the test method.
🧹 Proposed fix
- from unittest.mock import PropertyMock
-
d = self._create_device_config()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from unittest.mock import PropertyMock | |
| d = self._create_device_config() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/tests/test_controller.py` at line 279, The test
file contains an unused import "PropertyMock" in the import statement; remove
the unused import from openwisp_controller/config/tests/test_controller.py (the
line importing PropertyMock) so only required mocks are imported, ensuring no
other references to PropertyMock remain in the file.
Code Style and Commit Message FailuresHello @MichaelUray,
|
…p#1331 DeviceRegisterView._update_device_name calls ``device.skip_push_update_on_save()`` whenever an agent re-registers with a hostname different from the MAC address. That method is referenced in the view but is not implemented on the Device model, so every such re-registration crashes with HTTP 500: AttributeError: 'Device' object has no attribute 'skip_push_update_on_save' This breaks the consistent_registration / factory-reset workflow: 1. A router does a factory reset → loses its local UUID/key 2. The agent computes ``consistent_key = md5(MAC + "+" + secret)`` and POSTs ``/controller/register/``. 3. OpenWISP looks up the existing device by the consistent key and finds it. 4. The view enters the ``_update_device_name`` branch (because the hostname ``<MAC>-fwr-unprovisioned`` differs from the bare MAC). 5. The bogus method call raises ``AttributeError`` and the server returns HTTP 500. 6. The agent sees a response without the X-Openwisp-Controller header (the 500 error page does not set it) and aborts. 7. After 6 crash retries procd kills the agent. The router never recovers from a factory reset until an admin manually deletes the existing device record. This affects every deployment that uses consistent_registration and runs agents with hostnames that do not equal the bare MAC (e.g. anything that names devices by location or status). Wrap the call in ``getattr`` so the registration path succeeds whether the helper method exists or not. The actual helper can be reintroduced later without changing this site; the guard is forward-compatible. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MichaelUray
force-pushed
the
fix/register-skip-push-update-attribute-error
branch
from
b3e3f55 to
b0b93d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 553-561: Update the NOTE comment in controller/views.py to remove
the incorrect claim that skip_push_update_on_save() was never implemented on
Device; instead state that skip_push_update_on_save() is defined on
AbstractDevice (base/device.py) but the runtime guard around getattr(device,
"skip_push_update_on_save", None) is kept for defensive/backwards-compatibility
reasons (e.g., older subclasses or third-party devices may not implement it), so
leave the callable check and call to skip() unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d740da73-098b-46f8-8e9c-ea3cf2a30c29
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (4)
openwisp_controller/config/controller/views.py (2)
147-166: LGTM! Reconciliation hook is well-placed and documented.The grace period constant is clearly documented, and calling
_reconcile_modified_statusafter emittingchecksum_requestedensures the signal fires even if reconciliation fails.
171-236: LGTM! The reconciliation logic is well-designed with good performance characteristics.The fast-path optimization checking cached config status before hitting the DB is a good pattern. The docstring clearly explains the edge case being addressed. Error handling with
logger.exceptionensures failures don't crash the checksum endpoint while still being observable.openwisp_controller/config/tests/test_controller.py (2)
273-306: LGTM! Comprehensive test covering the reconciliation flow.Good approach using
Config.objects.filter().update()to backdate themodifiedtimestamp—this correctly bypasses Django'sauto_nowbehavior that would otherwise reset the timestamp on.save(). The test properly verifies both the grace period boundary and the final reconciled state.
308-328: LGTM! Good boundary condition coverage.These two tests correctly verify that:
- Configs already in "applied" status are not modified
- Configs within the grace period remain "modified"
Together with the main reconciliation test, this provides solid coverage of the feature.
Commit Message Format ViolationHello @MichaelUray, Your commit message is missing the issue number in the body. Fix: Here's an example of the correct format: |
DeviceRegisterView._update_device_name calls ``device.skip_push_update_on_save()`` whenever an agent re-registers with a hostname different from the MAC address. While the method is defined on AbstractDevice, subclasses or older installations that lack it would crash with AttributeError on every such re-registration (HTTP 500). This breaks the consistent_registration / factory-reset workflow: 1. A router does a factory reset and loses its local UUID/key. 2. The agent computes a consistent key and POSTs to /register/. 3. OpenWISP finds the existing device by key. 4. The view enters _update_device_name (hostname differs from MAC). 5. If skip_push_update_on_save is missing, AttributeError → HTTP 500. 6. The agent sees no X-Openwisp-Controller header and aborts. 7. After 6 retries procd kills the agent. Wrap the call in ``getattr`` so the registration path succeeds whether the helper method exists or not. The guard is forward-compatible. Closes openwisp#1331
MichaelUray
force-pushed
the
fix/register-skip-push-update-attribute-error
branch
from
b0b93d6 to
4c7c599
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 482-484: The current code calls skip = getattr(device,
"skip_push_update_on_save", None) and invokes skip() when callable but leaves
the non-callable/missing case silent; add an else branch that logs a warning via
the existing logger (or import the module logger) indicating the device is
missing skip_push_update_on_save to surface compatibility/runtime gaps during
re-registration—reference the device object and the attribute name
skip_push_update_on_save in the log message so operators can identify which
device/class triggered the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 269eb490-4ac5-4ac6-95e0-a56b652a2b1a
📒 Files selected for processing (1)
openwisp_controller/config/controller/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/controller/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/controller/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/controller/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/controller/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/controller/views.py
| skip = getattr(device, "skip_push_update_on_save", None) | ||
| if callable(skip): | ||
| skip() |
There was a problem hiding this comment.
Add warning log when fallback path is used
The guarded call is correct, but the missing-method case is an unusual runtime condition and is currently silent. Please log a warning in the else path so operators can detect compatibility gaps during re-registration.
Suggested patch
skip = getattr(device, "skip_push_update_on_save", None)
if callable(skip):
skip()
+ else:
+ logger.warning(
+ "Device %s does not implement skip_push_update_on_save(); "
+ "continuing registration without push-skip optimization.",
+ device.pk,
+ )As per coding guidelines: "New code must handle errors properly: ... log unusual conditions with warning level."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/controller/views.py` around lines 482 - 484, The
current code calls skip = getattr(device, "skip_push_update_on_save", None) and
invokes skip() when callable but leaves the non-callable/missing case silent;
add an else branch that logs a warning via the existing logger (or import the
module logger) indicating the device is missing skip_push_update_on_save to
surface compatibility/runtime gaps during re-registration—reference the device
object and the attribute name skip_push_update_on_save in the log message so
operators can identify which device/class triggered the fallback.
Commit Message Format FailureHello @MichaelUray, The commit message failed validation because the issue number ( Fix: Example of a correct commit message format: |
…p#1331 DeviceRegisterView._update_device_name calls ``device.skip_push_update_on_save()`` whenever an agent re-registers with a hostname different from the MAC address. While the method is defined on AbstractDevice, subclasses or older installations that lack it would crash with AttributeError on every such re-registration (HTTP 500). This breaks the consistent_registration / factory-reset workflow: 1. A router does a factory reset and loses its local UUID/key. 2. The agent computes a consistent key and POSTs to /register/. 3. OpenWISP finds the existing device by key. 4. The view enters _update_device_name (hostname differs from MAC). 5. If skip_push_update_on_save is missing, AttributeError → HTTP 500. 6. The agent sees no X-Openwisp-Controller header and aborts. 7. After 6 retries procd kills the agent. Wrap the call in ``getattr`` so the registration path succeeds whether the helper method exists or not. The guard is forward-compatible. Closes openwisp#1331
MichaelUray
force-pushed
the
fix/register-skip-push-update-attribute-error
branch
from
4c7c599 to
a250197
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/controller/views.py (1)
482-484:⚠️ Potential issue | 🟡 MinorLog the guarded fallback path with warning level.
At Line 482-Line 484, missing/non-callable
skip_push_update_on_saveis handled silently. Please add anelsewarning so operators can detect compatibility gaps during re-registration.Proposed patch
skip = getattr(device, "skip_push_update_on_save", None) if callable(skip): skip() + else: + logger.warning( + "Device %s does not implement skip_push_update_on_save(); " + "continuing registration without push-skip optimization.", + device.pk, + )As per coding guidelines: "New code must handle errors properly: ... log unusual conditions with warning level."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/controller/views.py` around lines 482 - 484, The current registration code checks skip = getattr(device, "skip_push_update_on_save", None) and only calls skip() when callable, but it silently ignores the non-callable/fallback case; update the block around skip_push_update_on_save to add an else branch that logs a warning via the controller's logger (include device identifier, e.g., device.pk or device.serial if available) indicating that skip_push_update_on_save is missing or not callable to surface compatibility gaps during re-registration; reference the existing variable names skip and device and place the warning at the same conditional scope where skip() is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 482-484: The current registration code checks skip =
getattr(device, "skip_push_update_on_save", None) and only calls skip() when
callable, but it silently ignores the non-callable/fallback case; update the
block around skip_push_update_on_save to add an else branch that logs a warning
via the controller's logger (include device identifier, e.g., device.pk or
device.serial if available) indicating that skip_push_update_on_save is missing
or not callable to surface compatibility gaps during re-registration; reference
the existing variable names skip and device and place the warning at the same
conditional scope where skip() is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95083df3-a8e1-4d2e-90ba-f57f6cab255e
📒 Files selected for processing (1)
openwisp_controller/config/controller/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/controller/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/controller/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/controller/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/controller/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/controller/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/controller/views.py
|
coverage: 98.66% (+0.07%) from 98.588% — MichaelUray:fix/register-skip-push-update-attribute-error into openwisp:master |
2 participants
Problem
DeviceRegisterView._update_device_namecallsdevice.skip_push_update_on_save()whenever an agent re-registers with a hostname that differs from the MAC address. That method is referenced in the view but is not implemented on theDevicemodel, so every such re-registration crashes with HTTP 500:Impact
This breaks the
consistent_registration/ factory-reset workflow:consistent_key = md5(MAC + "+" + secret)and POSTs/controller/register/key=consistent_keyand finds it_update_device_namebranch (because the hostname<MAC>-fwr-unprovisioneddiffers from the bare MAC)AttributeError→ server returns HTTP 500X-Openwisp-Controllerheader (the 500 error page doesn't have it) and abortsprocdkills the agentThe router never recovers from a factory reset until an admin manually deletes the existing device record. This affects every deployment that uses
consistent_registrationand runs agents with hostnames that don't equal the bare MAC (e.g. anything that names devices by location or status).Reproduction
Result before patch:
Fix
Wrap the call in
getattrso the registration succeeds whether the helper method exists or not. The actualskip_push_update_on_savemethod can be re-introduced separately without changing this site again — the guard is forward-compatible.Result after patch:
The device keeps its existing UUID and key (because
consistent_registrationis now able to find it), and the agent continues with the same configuration as before the reset.Compatibility
skip_push_update_on_saveis reintroduced on theDevicemodel later, the guard happily calls it.openwisp-configversion benefits immediately.Tested on
openwisp/openwisp-dashboard:25.10.2)